Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: remove optionally using operating system in the manifest file name hash #552

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

godobyte
Copy link
Contributor

@godobyte godobyte commented Jan 11, 2025

What was the problem/requirement? (What/Why)

attachment download and attachment upload commands support upload and download in a batch. They use the premise manifest file name contains the hash of the root path where the manifest was generated for to map manifest file to its corresponding root.

However, file_system_location_name is used in certain cases when passed in for some reason. This lead to the following problems at the time of downloading or uploading

  1. manifests and root paths approach has no visibility of file_system_location_name
  2. path mapping approach potentially has no visibility of file_system_location_name

What was the solution? (How)

By tracing the code, this change to optionally use file_system_location_name in hash was in #10. This is only used for output manifest name. There is no risk of hash collision that override the file since output manifest is partitioned at session action level (i.e. s3://XXXXX/Deadline/Manifests/farm-XXX/queue-XXX/job-XXX/step-XXX/task-XXX-0/2024-12-05T00:59:46.766370Z_sessionaction-313e8e3e1c67466684d28e2f8be07a38-1/08d256ef06279328acd95ac6a3d4f984_output, which has one and only one operating system. Output download discovers the manifests using formatted s3 partition.

Input manifest should also be unique to its root. Additionally, there is no hash check on the file name, which guarantees this change does not introduce regression.

Input manifest is already persisted as part of job

attachments:
  manifests:
  - rootPath: /bealine-clients/deadline-cloud-worker-agent/scripts/submit_jobs/asset_example
    rootPathFormat: posix
    outputRelativeDirectories:
    - .
    inputManifestPath: farm-XXX/queue-XXX/Inputs/43fc2f90cec04ba5a2aa3706c50fb786/0bb7eb91fdf8780c4a7e6174de6dfc5e_input

What is the impact of this change?

Output manifest file would not have file_system_location_name in the hash par of job name. This is to support the new approach for attachment commands.

How was this change tested?

  • Have you run the unit tests? [Y]
  • Ran integ tests [Y]
    • 28 passed, 2 skipped, 5 warnings in 74.34s (0:01:14)
  • Have you made changes to the download or asset_sync modules? If so, then it is highly recommended
    that you ensure that the docker-based unit tests pass. [Y]
    • 32 passed, 12 skipped in 14.59s

Was this change documented?

  • Are relevant docstrings in the code base updated? [N/A]
  • Has the README.md been updated? If you modified CLI arguments, for instance. [N/A]

Does this PR introduce new dependencies?

  • This PR adds one or more new dependency Python packages. I acknowledge I have reviewed the considerations for adding dependencies in DEVELOPMENT.md.
  • This PR does not add any new dependencies.

Is this a breaking change?

This PR changes the way manifest file name is generated to be the hash of root path.

Does this change impact security?

No


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@godobyte godobyte requested a review from a team as a code owner January 11, 2025 00:56
@jericht jericht marked this pull request as draft January 11, 2025 01:36
@jericht jericht added the job attachments For an issue with job attachments label Jan 13, 2025
@@ -224,16 +224,15 @@ def _gather_upload_metadata(
manifest: BaseAssetManifest,
source_root: Path,
manifest_name_suffix: str,
# TODO - remove file_system_location_name after ASSET_SYNC_JOB_USER_FEATURE completion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a tracking ticket so we don't forget?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we do, it's linked in the design doc.

Copy link

@godobyte godobyte marked this pull request as ready for review January 23, 2025 23:25
@godobyte godobyte requested a review from a team as a code owner January 23, 2025 23:25
@godobyte godobyte merged commit ed5c0cb into aws-deadline:mainline Jan 24, 2025
25 checks passed
@godobyte godobyte deleted the wa-integ branch January 24, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
job attachments For an issue with job attachments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants